-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connect to sage intacct #43661
Connect to sage intacct #43661
Conversation
Please ignore the typescript error for now as it's not really connected. I'll fix it later(I hope that it will be fixed by some other PR and I will just merge main 😃) |
src/pages/workspace/accounting/intacct/EnterSageIntacctCredentialsPage.tsx
Outdated
Show resolved
Hide resolved
key: policy.id, | ||
icon: policy.avatarURL ? policy.avatarURL : ReportUtils.getDefaultWorkspaceAvatar(policy.name), | ||
iconType: policy.avatarURL ? CONST.ICON_TYPE_AVATAR : CONST.ICON_TYPE_WORKSPACE, | ||
description: date ? `Sage Intacct - Last synced ${date}` : 'Sage Intacct', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
src/languages/en.ts
Outdated
if (!integrationToConnect) { | ||
return `Are you sure you want to disconnect ${currentIntegration || 'this integration'}?`; | ||
} | ||
return `Are you sure you want to disconnect ${currentIntegration || 'this integration'} to set up ${integrationToConnect}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have an alternative for the integrationToConnect
(in case it is undefined
)
src/languages/es.ts
Outdated
if (!integrationToConnect) { | ||
return `¿Estás seguro de que quieres desconectar ${currentIntegration || 'integración'}?`; | ||
} | ||
return `¿Estás seguro de que quieres desconectar ${currentIntegration || 'this integration'} para configurar ${integrationToConnect}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
src/libs/actions/Policy/Member.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to change this file 🤔 Are there any important changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fix for typescript error that also happens on main. When this type error will be fixed on main this changes will not be present on this PR.
src/libs/actions/Policy/Policy.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one seems unrelated to the PR (like the one above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
src/pages/workspace/accounting/intacct/EnterSageIntacctCredentialsPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/intacct/EnterSageIntacctCredentialsPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/intacct/EnterSageIntacctCredentialsPage.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page will be empty if there's no existing connection, I think we will need a placeholder or smth here 🤔
Or Should we disable Reuse existing connection
from three dot menu?
cc @yuwenmemon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no existing connection, then this page will never show up. User will be navigated directly to IntacctPrerequisitesPage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This's what I'm seeing, I still can open it from three dot menu
Screen.Recording.2024-06-19.at.09.10.09.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's on me, now with new condition this ThreeDotsMenu won't open when there's no existing Sage Intacct connection
src/pages/workspace/accounting/intacct/IntacctPrerequisitesPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/intacct/IntacctPrerequisitesPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/intacct/EnterSageIntacctCredentialsPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/intacct/ExistingConnectionsPage.tsx
Outdated
Show resolved
Hide resolved
We have conflicts here @SzymczakJ |
This comment was marked as resolved.
This comment was marked as resolved.
I got a loop while going back from Screen.Recording.2024-06-28.at.17.00.31.mov |
Fixed all comments and completed author checklist, I think this is ready for merge. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb ChromeiOS: Nativeios.moviOS: mWeb SafarimSafari.movMacOS: Chrome / Safarichrome.movchrome.another.movMacOS: Desktopdesk.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
SAGE_INTACCT_SYNC_IMPORT_EMPLOYEES: 'intacctImportEmployees', | ||
SAGE_INTACCT_SYNC_IMPORT_DIMENSIONS: 'intacctImportDimensions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: Why move this it was in alphabetical order already 🤔
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.0.4-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.0.4-5 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
const lastSuccessfulSyncDate = policy.connections?.intacct.lastSync?.successfulDate; | ||
const date = lastSuccessfulSyncDate ? datetimeToRelative(lastSuccessfulSyncDate) : undefined; | ||
return { | ||
title: policy.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a regression #45848, we have to add avatarID
as policy.id
so has same avatar color as the workspace.
@@ -143,8 +159,7 @@ function PolicyAccountingPage({policy, connectionSyncProgress}: PolicyAccounting | |||
const accountingIntegrations = Object.values(CONST.POLICY.CONNECTIONS.NAME).filter((name) => !(name === CONST.POLICY.CONNECTIONS.NAME.NETSUITE && !canUseNetSuiteIntegration)); | |||
const connectedIntegration = accountingIntegrations.find((integration) => !!policy?.connections?.[integration]) ?? connectionSyncProgress?.connectionName; | |||
const policyID = policy?.id ?? '-1'; | |||
const successfulDate = policy?.connections?.quickbooksOnline?.lastSync?.successfulDate; | |||
const formattedDate = useMemo(() => (successfulDate ? new Date(successfulDate) : new Date()), [successfulDate]); | |||
const successfulDate = getIntegrationLastSuccessfulDate(connectedIntegration ? policy?.connections?.[connectedIntegration] : undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR caused a regression here - #45780
We should get the last successful date of the integration. Then, if connectionSyncProgress
is the same integration displayed and the state is 'jobDone', get the more recent update time of the two.
Details
Fixed Issues
$ #43532
PROPOSAL: N/A
Tests
Verify that Enter Credentials page looks like this:
Connect to Sage Intacct // right now we can connect on Old Dot
Create another workspace and try connecting it to Sage Intacct
Verify that you pop up/modal with “Create new connection” and “Reuse existing connection” options pops up
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-06-28.at.13.44.06.mov
Android: mWeb Chrome
anotherandroid.mov
iOS: Native
ios.mov
iOS: mWeb Safari
mwebIOS1.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov